Skip to content

[SPARK-59438][SQL][TESTS] Add planner-stage test coverage for ASOF JOIN sort-merge operator - #58742

Open
LukaZdravic wants to merge 6 commits into
apache:masterfrom
LukaZdravic:SPARK-59438-asof-planner-tests
Open

LukaZdravic wants to merge 6 commits into
apache:masterfrom
LukaZdravic:SPARK-59438-asof-planner-tests

Conversation

@LukaZdravic

@LukaZdravic LukaZdravic commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Test-only change. It adds planner-stage (physical-planning) coverage for the ASOF JOIN
sort-merge operator. No production code changes.

  • PlannerSuite: two tests build a SortMergeAsOfJoinExec, run EnsureRequirements, and
    check requiredChildDistribution:
    • No equi-keys: both sides shuffle to a single partition (the AllTuples arm).
    • With equi-keys: both sides hash-partition on the key (the ClusteredDistribution arm).
  • SortMergeAsOfJoinSuite: one test checks that a null-safe equi-key (<=>) in ON
    becomes a residual condition, not an equi-key. Rows whose key is NULL on both sides then match. This is the counterpart to the EqualTo case (NULL keys never match) and mirrors InnerJoinSuite's "inner join, null safe" test.

No production code is changed.

Why are the changes needed?

The AsOfJoinSelection strategy and SortMergeAsOfJoinExec operator were tested end-to-end for result correctness, but two planner-stage behaviors had no direct test:

  1. the operator's requiredChildDistribution (single partition when there are no
    equi-keys, hash-partition on the key otherwise), and
  2. the strategy routing EqualNullSafe (<=>) in ON to the residual condition instead
    of an equi-key.
    Sibling join operators cover the same behaviors. This brings ASOF JOIN to the same bar.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

New unit tests. Ran PlannerSuite and SortMergeAsOfJoinSuite locally (via Maven); all tests pass.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code

…IN sort-merge operator

Adds two planner-stage tests for the ASOF JOIN sort-merge path, both
mirroring existing sibling-join test patterns:

- `PlannerSuite`: a no-equi-key `AsOfJoin` requires a single partition on
  both sides (the `AllTuples` branch of `SortMergeAsOfJoinExec`'s
  `requiredChildDistribution`), verified by running `EnsureRequirements`
  and asserting a single-partition shuffle on each side. Follows the
  existing `SortMergeJoinExec` distribution tests (SPARK-24495 /
  SPARK-27485).
- `SortMergeAsOfJoinSuite`: a null-safe (`<=>`) equi-key in `ON` is routed
  to the residual condition rather than treated as an equi-key, so null
  keys on both sides do match -- the counterpart to the existing `EqualTo`
  case where null keys never match. Mirrors `InnerJoinSuite`'s
  "inner join, null safe".

Test-only change; no production code is modified.
Copilot AI lite review requested due to automatic review settings September 11, 2026 13:56
@LukaZdravic
LukaZdravic marked this pull request as draft September 11, 2026 13:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Test-only changes with no unresolved issues.

Pull request overview

Adds test-only planner and execution coverage for ASOF JOIN behavior.

Changes:

  • Tests single-partition requirements for no-equi-key joins.
  • Tests null-safe conditions matching null keys.
File summaries
File Summary
sql/core/src/test/scala/org/apache/spark/sql/SortMergeAsOfJoinSuite.scala Adds null-safe ASOF JOIN coverage.
sql/core/src/test/scala/org/apache/spark/sql/execution/PlannerSuite.scala Adds distribution requirement coverage.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…l path

The null-safe (<=>) as-of test gave each left row one null candidate, so it could
not tell "keep closest" from "keep first" on the residual path. Reuse the EqualTo
sibling's data (only the ON predicate differs) and add a null-key left row at
ts=10 with two null candidates (ts=3, ts=8). The scanner must keep the closest
(ts=8), so a keep-first regression now fails.
…qui-key

The null-safe (<=>) as-of test checked only output rows, which pass whether <=>
is routed to the residual condition or treated as a null-safe equi-key. Inspect
the executed plan and assert the SortMergeAsOfJoinExec has empty equi-keys, so
the test verifies the routing its comment describes.
…t comments

Address review feedback:
- Add the equi-key counterpart to the no-equi-key distribution test, asserting
  each side hash-partitions on the key rather than shuffling to one partition.
- Assert requiredChildOrdering in both distribution tests (keys then sort exprs).
- Shorten the <=> and distribution test comments to one line each.
@LukaZdravic
LukaZdravic requested a review from vranes September 14, 2026 13:12
…n both tests

The distribution was checked only through EnsureRequirements, but the
if (leftKeys.isEmpty) AllTuples else ClusteredDistribution branch is the
behavior under test. Assert it directly: AllTuples with no equi-keys, and
ClusteredDistribution on the key with equi-keys.
… just its type

The equi-key test asserted the shuffle outputPartitioning isInstanceOf
HashPartitioning, a type-only check. Assert its expressions equal Seq(exprC)
so a hash on a different column fails the test.
@LukaZdravic
LukaZdravic marked this pull request as ready for review September 14, 2026 13:43

@vranes vranes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@uros-b

uros-b commented Sep 14, 2026

Copy link
Copy Markdown
Member

Thank you @LukaZdravic and @vranes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants